feat(migrate): CommonChunksPlugin to SplitChunksPlugin by dhruvdutt · Pull Request #558 · webpack/webpack-cli

dhruvdutt · 2018-07-31T19:17:45Z

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?
Yes

If relevant, did you update the documentation?
No

Summary
Initial implementation for migration CCP to SCP.

It is not possible to map all the options/properties from CCP to SCP migration. It's best to do a basic migration optimization.splitChunks object migration and give a link to docs, migration guide and allow the user the flexibility to make more adjustments/optimizations with newer options available in SCP. Also, many common use-cases directly work out of the box in the new SCP plugin.

TODO:

TBD cases

Does this PR introduce a breaking change?
No

Other information

Closes #393

evenstensberg

Left some comments and I think @sokra should be looking at the end output here too

evenstensberg · 2018-08-01T11:45:51Z

+	pluginProps.forEach((p: INode) => {
+		switch (p.key.name) {
+			case "names":
+				p.value.elements.forEach((chunkName) => {


returns void

evenstensberg · 2018-08-01T11:46:50Z

+		];
+		switch (chunkName.value) {
+			case "vendor":
+				return j.property(


this is not always the case, user supplies the regex

Can you please give an example syntax of how the user supplies the regex? I can't find it in the CommonsChunkPlugin docs

If user supplies the test prop they can regex chunks

I can't find that option in the older CommonsChunkPlugin docs. test is supported in SplitChunksPlugin docs.

No need to take review comments for actual comments. A regex to match a chunk was possible in v.2 if I recall right. @sokra needs to guide you here, I don't fully know backwards compat spec

evenstensberg · 2018-08-01T11:47:02Z

+
+	// create chunk cache group option
+	function createChunkCache(chunkName) {
+		const commonProperties = [


missing type

evenstensberg · 2018-08-01T11:47:19Z

@@ -0,0 +1,3 @@
+[*]


evenstensberg · 2018-08-01T11:58:40Z

If user supplies the test prop they can regex chunks

webpack-bot · 2018-08-13T10:07:55Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

sokra · 2018-08-15T08:25:48Z

+                    minChunks: 1,
+                    test: 'vendor'
+                }
+            }


looks correct to me

sokra · 2018-08-15T08:28:09Z

+                    name: 'commons',
+                    chunks: 'initial',
+                    enforce: true,
+                    minChunks: 1,


minChunks should be the number of defined entrypoints.

sokra · 2018-08-15T08:30:44Z

+    optimizations: {
+        splitChunks: {
+            chunks: 'async',
+            minSize: 2000,


I would move this into the cacheGroups.main

Still not resolved.

This case is not 100% map-able to splitChunks. This might be the best option:

cacheGroups: { main: { minSize: 2000, chunks: 'async' } }

sokra · 2018-08-15T08:32:27Z

+                    enforce: true,
+                    minChunks: 1,
+                    test: 'main'
+                }


looks correct

sokra · 2018-08-15T08:34:32Z

+                    enforce: true,
+                    minChunks: 1,
+                    test: 'main',
+                    test: ({ resource }) => /node_modules/.test(resource)


You can't have test multiple times in the config.

You could be merged this way:

test: module => { if(module.getChunks().some(chunk => chunk.name === 'main')) return true; const fn = ({ resource }) => /node_modules/.test(resource); return fn(module); }

sokra · 2018-08-15T08:34:44Z

+                    minChunks: 1,
+                    test: 'main',
+
+                    test: ({ resource }) => {


sokra · 2018-08-15T08:34:48Z

+                    minChunks: 1,
+                    test: 'main',
+
+                    test: function ({ resource }) {


sokra · 2018-08-15T08:34:54Z

+                    minChunks: 1,
+                    test: 'main',
+
+                    test: function ({ resource }) {


sokra · 2018-08-20T07:50:35Z

+            chunks: 'async',
+
+            cacheGroups: {
+                minSize: 2000,


This is incorrect and will fail the validation.

sokra · 2018-08-20T07:52:09Z

+
+                        const fn = ({ resource }) => /node_modules/.test(resource);
+                        return fn(module);
+                    }


Looks correct to me

sokra · 2018-08-20T07:52:43Z

+                        };
+
+                        return fn(module);
+                    }


Looks correct to me

sokra · 2018-08-20T07:52:55Z

+
+                        return fn(module);
+                    }
+                }


Looks correct to me

sokra · 2018-08-20T07:53:07Z

+                        };
+
+                        return fn(module);
+                    }


Looks correct to me

sokra · 2018-08-20T07:53:22Z

+    optimizations: {
+        splitChunks: {
+            cacheGroups: {
+                minSize: 3000,


sokra · 2018-08-20T07:53:58Z

+                    name: 'main',
+                    chunks: 'initial',
+                    enforce: true,
+                    minChunks: 1,


You don't need minChunks: 1 when using enforce: true

sokra · 2018-08-20T07:55:23Z

+                    minChunks: 2,
+                    test: 'vendor'
+                }
+            }


In this case runtimeChunk: { name: "vendor" } makes sense.

The input webpack config doesn't have runtime anywhere. How to identify when to apply runtimeChunk?

This is the input case for this output:

module.exports = { entry: { app: './src/app.js', vendor: './src/vendors.js', }, plugins: [ new webpack.optimize.CommonsChunkPlugin({ names: ["app", "vendor"], minChunks: 2 }) ] }

sokra · 2018-08-20T07:56:06Z

+                    name: 'vendor',
+                    chunks: 'initial',
+                    enforce: true,
+                    minChunks: 2,


minChunks: 2 is incorrect here.

When using test: "string", minChunks should not be set.

sokra

A few minor issues, otherwise it's good enough...

sokra · 2018-08-20T20:08:53Z

+    optimizations: {
+        splitChunks: {
+            chunks: 'async',
+            minSize: 2000,


Still not resolved.

This case is not 100% map-able to splitChunks. This might be the best option:

cacheGroups: { main: { minSize: 2000, chunks: 'async' } }

sokra · 2018-08-20T20:10:24Z

+
+    optimizations: {
+        splitChunks: {
+            minSize: 3000,


This need to be moved into cacheGroups.main

evenstensberg

lgtm

webpack-bot added PR: unreviewed PR: CI-not-ok labels Jul 31, 2018

dhruvdutt force-pushed the migrate branch from c914b8a to e46a6b5 Compare July 31, 2018 19:26

webpack deleted a comment from webpack-bot Jul 31, 2018

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Jul 31, 2018

dhruvdutt force-pushed the migrate branch 3 times, most recently from a8ebc1d to e4fb993 Compare August 1, 2018 10:46

dhruvdutt changed the title ~~[WIP] feat(migrate): commonChunksPlugin to SplitChunksPlugin~~ [WIP] feat(migrate): CommonChunksPlugin to SplitChunksPlugin Aug 1, 2018

dhruvdutt force-pushed the migrate branch from e4fb993 to bc36f5a Compare August 1, 2018 10:55

dhruvdutt requested a review from evenstensberg August 1, 2018 10:55

evenstensberg requested changes Aug 1, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Aug 1, 2018

dhruvdutt force-pushed the migrate branch from bc36f5a to 0b8e51e Compare August 1, 2018 11:57

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Aug 1, 2018

dhruvdutt force-pushed the migrate branch from 5cd57c8 to 36fe9cc Compare August 7, 2018 12:46

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Aug 7, 2018

dhruvdutt added 5 commits August 7, 2018 18:34

feat(migrate): CCP to SCP - init

ecf8679

feat(migrate): CCP to SCP - runtime chunk

3d9d31c

feat(migrate): CCP to SCP - add enforce, chunk props

303857b

feat(migrate): CCP to SCP - fix cacheGroups

b4cdd03

feat(migrate): CCP to SCP - regex support, minChunks, async chunks

6329aaa

dhruvdutt force-pushed the migrate branch 2 times, most recently from ebf2a98 to 846cc5a Compare August 7, 2018 13:12

feat(migrate): CCP to SCP - fix tests

bde0049

webpack-bot added the PR: CI-ok label Aug 13, 2018

sokra requested changes Aug 15, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Aug 15, 2018

feat(migrate): CCP to SCP - merge test entry prop and function exp

1e441e3

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Aug 16, 2018

sokra requested changes Aug 20, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Aug 20, 2018

feat(migrate): CCP to SCP - fix minSize issue

b68a7d0

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Aug 20, 2018

feat(migrate): CCP to SCP - fix minChunks, test issue

e404f3b

dhruvdutt force-pushed the migrate branch from 60099c9 to e404f3b Compare August 20, 2018 13:21

sokra requested changes Aug 20, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested PR: review-outdated and removed PR: review-outdated PR: reviewed-changes-requested labels Aug 20, 2018

dhruvdutt force-pushed the migrate branch from b751e62 to e404f3b Compare August 20, 2018 20:18

feat(migrate): CCP to SCP - fix minSize, async chunk

2d63478

dhruvdutt changed the title ~~[WIP] feat(migrate): CommonChunksPlugin to SplitChunksPlugin~~ feat(migrate): CommonChunksPlugin to SplitChunksPlugin Aug 20, 2018

evenstensberg approved these changes Aug 21, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Aug 21, 2018

evenstensberg merged commit 8fa176f into webpack:master Aug 21, 2018

dhruvdutt deleted the migrate branch August 21, 2018 05:42

snyk-bot mentioned this pull request Sep 12, 2021

[Snyk] Fix for 1 vulnerabilities enterstudio/webpack-cli#20

Open

Uh oh!

Uh oh!

Conversation

dhruvdutt commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evenstensberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evenstensberg commented Aug 1, 2018

Uh oh!

webpack-bot commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sokra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvdutt Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evenstensberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

dhruvdutt commented Jul 31, 2018 •

edited

Loading

dhruvdutt Aug 20, 2018 •

edited

Loading